Introduce ability to specify which timezones to include in the native imagee… #1819
Conversation
|
Hello Jaikiran Pai, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jaikiran -(dot)- pai -(at)- gmail -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
Please check for the user |
|
Jaikiran Pai has signed the Oracle Contributor Agreement (based on email address jaikiran -(dot)- pai -(at)- gmail -(dot)- com) so can contribute to this repository. |
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/TimeZoneSubstitutions.java
Outdated
Show resolved
Hide resolved
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/TimeZoneSubstitutions.java
Outdated
Show resolved
Hide resolved
| @@ -78,12 +78,23 @@ public static TimeZone getTimeZone(String id) { | |||
| static class Options { | |||
| @Option(help = "When true, all time zones will be pre-initialized in the image.")// | |||
| public static final HostedOptionKey<Boolean> IncludeAllTimeZones = new HostedOptionKey<>(false); | |||
There was a problem hiding this comment.
Not sure this is the right default value. How much does -H:+IncludeAllTimeZones cost in terms of image size?
Anyhow, this is out of the scope of this PR.
There was a problem hiding this comment.
How much does -H:+IncludeAllTimeZones cost in terms of image size?
We have been having this similar conversation in the Quarkus project https://groups.google.com/forum/#!msg/quarkus-dev/J1afzAhpShI/A5vmUGJLAQAJ
My experiments (with a couple of different programs, one being the HelloWorld example used in substratevm project) have shown that when IncludeAllTimeZones isn't set then the filesize is something along lines of 2.9M:
ls -lh helloworld
-rwxr-xr-x 1 jaikiran jaikiran 2.9M Nov 14 23:22 helloworld
and when you set IncludeAllTimeZones, the size increases very slightly to:
-rwxr-xr-x 1 jaikiran jaikiran 3.3M Nov 14 23:50 ./helloworld
So not a big difference really. So yes, by default including all timezones probably will help application frameworks like Quarkus, where in certain situations there isn't really an easy way to determine at build time which timezones will be needed in the native image (imagine an application which communicates with a DB server whose timezone information can only be obtained at runtime).
But yes, like you say, the default value of IncludeAllTimeZones is something that needs to be discussed and sorted out separately.
| @@ -76,18 +76,27 @@ public static TimeZone getTimeZone(String id) { | |||
| @AutomaticFeature | |||
| final class TimeZoneFeature implements Feature { | |||
| static class Options { | |||
| private static final TimeZone defaultZone = TimeZone.getDefault(); | |||
There was a problem hiding this comment.
Not a fan of static state, but I guess this one is OK to stay here.
|
I confirm too, I have the bug here: quarkiverse/quarkus-univocity-parsers@5321516 the timezone is different between my non-native test phase and the native test phase. I think the "-H:+IncludeAllTimeZones" isn't take in account |
Currently when a native-image is built it by defaults adds only
GMT,UTCand the default zone id as the timezones that are initialized and made available in the generated image. Of course, there'sIncludeAllTimeZonesoption which includes all the available timezones into the target image.However, it would be better to expose an additional option which lets users decide which specific timezones to include in the target image. Quarkus project has seen issues like these[1] where even a
GMTexpressed as a different idEtc/GMTisn't available in the target image. Of course, that's just one use case. Having a more finer control on which timezones get included in the target image will help users in configuring the native-image build accordingly or allowing frameworks like Quarkus an ability to add them intelligently.This is my first contribution to the graal project. However, I have a signed and approved OCA (as part of the JDK project) and here's my record in the census https://openjdk.java.net/census#jpai
I have done manual tests with these changes to verify that using the new
IncludeTimeZonesoption indeed includes the specific timezones in the target image and also made sure the exisitngIncludeAllTimeZonesoption isn't regressed. I am willing to add an automated test case (couldn't find documentation for it) if someone can guide me or point me to how to do that.[1] quarkusio/quarkus#5269